module: allowed module location list #34379

bzoz · 2020-07-15T12:28:27Z

Adds --allowed-module-location option that can be used to limit the paths Node.js is allowed to load modules from.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Adds --allowed-module-location option that can be used to limit the paths Node.js is allowed to load modules from. Fixes: nodejs#34124

richardlau · 2020-07-15T15:07:58Z

Does it make sense to have this as part of Policies (see also proposed extensions in #33504)?

This also doesn't appear to affect (intentionally? in which case it should probably be made clearer) ES modules.
cc @nodejs/modules @nodejs/security

bmeck · 2020-07-15T15:26:30Z

I think keeping this in policies makes sense, it essentially is a scoping mechanism that doesn't currently exist in the policies configuration. Currently policies must whitelist all permissions for loading files and the escape hatches to allow arbitrary loading are not currently specified to allow arbitrary folders. I also have concerns about symlink escapes if the idea is that you cannot access files outside of a directory we need to write up a design doc on symlinks to write down the reasoning.

Likely this can be approached in a few ways if we want to put it in the policy file, but I actually think matching "imports" and "exports" design from package.json would be ideal. Detecting a resource with a key with a trailing slash is treated as a scope. I am a bit unclear on what we want to put in the value itself but for now limiting it to {integrity: true, dependencies: true} seems adequate. I'm thinking in the case of collision the longest substring policy would be applied without any sort of cascading. I can also imaging setting "dependencies" properly, but am unclear on if we ever would want to allow an integrity list.

addaleax

We should be clear on what guarantees we’re making here.

As far as I can tell, this currently:

Only affects CommonJS
Assumes unmodified built-ins and Node.js internals
Seems like it would give odd results if the current path contains a symbolic link

Generally, I don’t think this is the kind of solution we should be aiming for when it comes to #34124, and I feel strongly that we should remove the Fixes: tag here. Any kind of opt-in restriction does not solve that issue reasonably, imo.

addaleax · 2020-07-15T15:51:17Z

doc/api/cli.md

@@ -64,6 +64,18 @@ If this flag is passed, the behavior can still be set to not abort through
 [`process.setUncaughtExceptionCaptureCallback()`][] (and through usage of the
 `domain` module that uses it).

+### `--allowed-module-location`
+<!-- YAML
+added: REPLACE_ME


Suggested change

added: REPLACE_ME

added: REPLACEME

bmeck · 2020-07-16T13:31:41Z

lib/internal/modules/cjs/loader.js

@@ -84,13 +84,15 @@ const preserveSymlinksMain = getOptionValue('--preserve-symlinks-main');
 const manifest = getOptionValue('--experimental-policy') ?
  require('internal/process/policy').manifest :
  null;
+const allowedModuleLocations = getOptionValue('--allowed-module-location');


should cache builtins to avoid mutations from affecting this (comment 1 of n)

Suggested change

const allowedModuleLocations = getOptionValue('--allowed-module-location');

const allowedModuleLocations = getOptionValue('--allowed-module-location');

const { relative: PathRelative, isAbsolute: PathIsAbsolute } = path;

bmeck · 2020-07-16T13:32:52Z

lib/internal/modules/cjs/loader.js

+  if (allowedModuleLocations && allowedModuleLocations.length > 0) {
+    locationAllowed = false;
+    debug('Allowed module locations count: %d', allowedModuleLocations.length);
+    for (const allowedLocation of allowedModuleLocations) {
+      const relative = path.relative(allowedLocation, filename);
+      debug('Relative path from allowed location "%s" to loaded "%s": %s',
+            allowedLocation, filename, relative);
+      if (relative === '' ||
+          (!relative.startsWith('..') && !path.isAbsolute(relative))) {
+        debug('Module will be allowed');
+        locationAllowed = true;
+        break;
+      }
+    }
+  }


should cache builtins to avoid mutations from affecting this (comment 2 of n)

Suggested change

if (allowedModuleLocations && allowedModuleLocations.length > 0) {

locationAllowed = false;

debug('Allowed module locations count: %d', allowedModuleLocations.length);

for (const allowedLocation of allowedModuleLocations) {

const relative = path.relative(allowedLocation, filename);

debug('Relative path from allowed location "%s" to loaded "%s": %s',

allowedLocation, filename, relative);

if (relative === '' ||

(!relative.startsWith('..') && !path.isAbsolute(relative))) {

debug('Module will be allowed');

locationAllowed = true;

break;

}

}

}

if (allowedModuleLocations && allowedModuleLocations.length > 0) {

locationAllowed = false;

debug('Allowed module locations count: %d', allowedModuleLocations.length);

// avoids mutation of Array.prototype[Symbol.iterator] from affecting this

for (let i = 0; i < allowedModuleLocations.length; i++) {

const allowedLocation = allowedModuleLocations[i];

const relative = PathRelative(allowedLocation, filename);

debug('Relative path from allowed location "%s" to loaded "%s": %s',

allowedLocation, filename, relative);

if (relative === '' ||

(!StringPrototypeStartsWith(relative, '..') && !PathIsAbsolute(relative))) {

debug('Module will be allowed');

locationAllowed = true;

break;

}

}

}

bmeck · 2020-07-16T13:47:13Z

in order for this approach to be applicable to the ESM loader this PR needs to put a change into:

the default loader implementation of resolve, see https://github.com/nodejs/node/blob/master/lib/internal/modules/esm/resolve.js#L766
it should also be applied to the package JSON reader

node/lib/internal/modules/package_json_reader.js

Line 17 in a55b77d

const [string, containsKeys] = internalModuleReadJSON(path);

(apparently even policies aren't covering that at the right timing looking at it)

Per symlinks, I think we could state that preventing symlink traversal isn't an issue unless people use --preserve-symlinks due to realpathing any escape should be on the realpath and be prevented looking at this code. Perhaps @zkochan or @arcanis has seen some model for what to do for symlinks escaping a directory in the real world already?

arcanis · 2020-07-16T13:59:52Z

I'm not sure that this approach would be an adequate fix. Many tools use variants of the resolution algorithm that Node has no control over - especially because the resulting modules aren't directly required.

For example let's take Webpack: even with this PR and the right flag set, nothing would prevent Webpack from bundling code from outside the allowed module tree, since its resolution goes through enhanced-resolve. Fixing it would require to make changes in the third-party project, but that's only one amongst many... Rollup does the same thing with yet another resolver.

Imo a better fix (albeit harder to implement) would be to provide tools for Node to prevent all filesystem calls to some parts of the filesystem.

bmeck · 2020-07-16T14:13:17Z

@arcanis I think fs calls and module loading are fundamentally different due to having different overall affects, e.g. you might want to allow access to a ./uploads directory but never want to load code from it.

bmeck · 2020-07-16T14:16:28Z

I'd also like this to remain scoped to how node performs actions, not how 3rd party tools do things like bundling. Module loading hooks are not fired for webpack's bundling workflows etc since those workflows do not want to actually evaluate the code they are bundling.

zkochan · 2020-07-22T19:14:53Z

@bmeck the latest version of pnpm uses symlinks that don't escape the node_modules directory. All realpaths of symlinks are inside the node_modules/.pnpm directory.

aduh95 · 2020-11-17T15:00:21Z

@bzoz do you plan to keep working on this?

bzoz · 2020-11-17T15:35:21Z

@aduh95 no, whit the feedback received I don't think this PR is a good aproach.

aduh95 · 2020-11-17T15:37:45Z

OK, closing it for now.

judy-fei · 2022-08-19T02:10:23Z

it seems --allowed-module-location has been removed, right? because i cannot search this flag in the latest nodejs, and i also try to add env variable NODE_ALLOWED_MODULE_LOCATIONS, but it seems not work for this issue, node still search all node_modules for parent folders.

ljharb · 2022-08-19T04:14:07Z

It was never added in the first place.

judy-fei · 2022-08-19T04:27:45Z

thanks liharb! so how to fix this issue?

ljharb · 2022-08-19T15:02:11Z

There is no "fix". node will always search upwards for every node_modules dir, at this time.

bmeck · 2022-08-19T15:36:27Z

See #43828 and related comments. Policies may serve your needs or that PR if/when it lands.

…

On Fri, Aug 19, 2022, 10:02 AM Jordan Harband ***@***.***> wrote: There is no fix. node will always search upwards for every node_modules dir. — Reply to this email directly, view it on GitHub <#34379 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABZJIZW5ENJZZYO5AIDM73VZ6OX7ANCNFSM4O2PR3BA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

module: allowed module location list

b7eb028

Adds --allowed-module-location option that can be used to limit the paths Node.js is allowed to load modules from. Fixes: nodejs#34124

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. errors Issues and PRs related to JavaScript errors originated in Node.js core. labels Jul 15, 2020

addaleax reviewed Jul 15, 2020

View reviewed changes

bmeck reviewed Jul 16, 2020

View reviewed changes

aduh95 closed this Nov 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

module: allowed module location list #34379

module: allowed module location list #34379

bzoz commented Jul 15, 2020

richardlau commented Jul 15, 2020

bmeck commented Jul 15, 2020

addaleax left a comment

addaleax Jul 15, 2020

bmeck Jul 16, 2020

bmeck Jul 16, 2020

bmeck commented Jul 16, 2020

arcanis commented Jul 16, 2020

bmeck commented Jul 16, 2020

bmeck commented Jul 16, 2020

zkochan commented Jul 22, 2020

aduh95 commented Nov 17, 2020

bzoz commented Nov 17, 2020

aduh95 commented Nov 17, 2020

judy-fei commented Aug 19, 2022

ljharb commented Aug 19, 2022

judy-fei commented Aug 19, 2022

ljharb commented Aug 19, 2022 •

edited

Loading

bmeck commented Aug 19, 2022 via email

	const allowedModuleLocations = getOptionValue('--allowed-module-location');
	const allowedModuleLocations = getOptionValue('--allowed-module-location');
	const { relative: PathRelative, isAbsolute: PathIsAbsolute } = path;

module: allowed module location list #34379

module: allowed module location list #34379

Conversation

bzoz commented Jul 15, 2020

Checklist

richardlau commented Jul 15, 2020

bmeck commented Jul 15, 2020

addaleax left a comment

Choose a reason for hiding this comment

addaleax Jul 15, 2020

Choose a reason for hiding this comment

bmeck Jul 16, 2020

Choose a reason for hiding this comment

bmeck Jul 16, 2020

Choose a reason for hiding this comment

bmeck commented Jul 16, 2020

arcanis commented Jul 16, 2020

bmeck commented Jul 16, 2020

bmeck commented Jul 16, 2020

zkochan commented Jul 22, 2020

aduh95 commented Nov 17, 2020

bzoz commented Nov 17, 2020

aduh95 commented Nov 17, 2020

judy-fei commented Aug 19, 2022

ljharb commented Aug 19, 2022

judy-fei commented Aug 19, 2022

ljharb commented Aug 19, 2022 • edited Loading

bmeck commented Aug 19, 2022 via email

ljharb commented Aug 19, 2022 •

edited

Loading